Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Header Link Pagination #622

Merged
merged 8 commits into from
Nov 29, 2018

Conversation

natanfelles
Copy link
Contributor

@natanfelles natanfelles commented Jul 21, 2017

Example Method:

public function listAll($user_id)
{
    $usersCourses = $this->usersCoursesModel->asArray()
                                            ->where('user_id', $user_id)
                                            ->paginate(2);

    //$pager = $this->usersCoursesModel->pager;
    //echo $pager->links('default', 'default_header');

    $uc = [];
    foreach ($usersCourses as $cours)
    {
        $this->userCoursesEntity = new UserCoursesEntity();
        $this->userCoursesEntity->fill($cours);
        $uc[] = $this->userCoursesEntity;
    }
    
    return $this->response->setStatusCode(200)
                          ->setContentType('application/json')
                          ->setDate(new \DateTime())
                          ->setCache(['max-age' => 300, 'etag' => 'kksdhfn'])
                          // Link Header: http://tools.ietf.org/html/rfc5988
                          ->setLink($this->usersCoursesModel->pager)
                          ->setBody(json_encode($uc));
}

Response Header:

GET http://localhost:8080/users/72/courses?page=3

HTTP/1.1 200 OK
Host: localhost:8080
Connection: close
Content-Type: application/json; charset=UTF-8
Date: Fri, 21 Jul 2017 14:42:10 GMT
ETag: kksdhfn
Cache-control: max-age=300
Link: <http://localhost:8080/users/72/courses?page=1>; rel="first",<http://localhost:8080/users/72/courses?page=2>; rel="prev",<http://localhost:8080/users/72/courses?page=4>; rel="next",<http://localhost:8080/users/72/courses?page=7>; rel="last"

https://tools.ietf.org/html/rfc5988
https://developer.github.com/v3/#pagination

@@ -19,7 +19,8 @@ class Pager extends BaseConfig
*/
public $templates = [
'default_full' => 'CodeIgniter\Pager\Views\default_full',
'default_simple' => 'CodeIgniter\Pager\Views\default_simple'
'default_simple' => 'CodeIgniter\Pager\Views\default_simple',
'default_header' => 'CodeIgniter\Pager\Views\default_header'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this can be hidden.

Calling 'CodeIgniter\Pager\Views\default_header' directly in $pager->links.

@lonnieezell
Copy link
Member

Looking it over, but as an initial complete aside - you can set the Model's returnType to the class name you want returned, and save you the effort of populating the entities yourself. So, in your model:

protected $returnType = 'App\Entities\UserCoursesEntity';

@natanfelles
Copy link
Contributor Author

Uhmm. Thanks! Day of testing here. 😄

What about the $this->response->setLink()?

@lonnieezell
Copy link
Member

The problem I"m having so far is that the small handful of documents that I've Googled use stylesheets as examples, or resources in an Atom feed. Neither of those are specific to the pager. While I'm not opposed to adding the setLink() on the response, it should be generic enough to work in all of the intended use cases. If that can't be done, then there's no need for a specific call built into the framework.

To be honest, in the decade I've been building apps I've never had a need for this header, so it sounds like maybe something left to app-specific usage. You can always make a custom method in the controller or even use your own Response class that extends the built in one.

@natanfelles
Copy link
Contributor Author

Got it. I'm very new to REST. I made this inspired by the GitHub API documentation and thought it would be useful in most cases.

@lonnieezell
Copy link
Member

I'll have to think on the formatting later, but something built into the API/ReponseTrait dealing with the paging could be very helpful.

@louisl
Copy link
Contributor

louisl commented Jul 28, 2017

I'm modifying a CI3 api at the moment where I need to implement this somehow. For mobile apps especially, all the little savings in processing add up. I haven't tested natanfelles implementation here but it's a feature I'd use if it were available.

@natanfelles
Copy link
Contributor Author

@natanfelles
Copy link
Contributor Author

If something like that will be implemented, it could be done on the fly:

/**
 * Get pagination links for head - SEO
 *
 * @param \CodeIgniter\Pager\Pager|null $pager
 *
 * @see https://webmasters.googleblog.com/2011/09/pagination-with-relnext-and-relprev.html
 *
 * @return string
 */
public function renderPaginationLinks(\CodeIgniter\Pager\Pager $pager = null):string
{
	if ($pager instanceof \CodeIgniter\Pager\Pager)
	{
		$links = PHP_EOL;

		// Previous
		if ($pager->getPreviousPageURI())
		{
			$links .= '<link rel="prev" href="'. $pager->getPreviousPageURI() . '">' . PHP_EOL;
		}

		// Current
		$links .= '<link rel="canonical" href="'. $pager->getPageURI($pager->getDetails()['currentPage']) . '">' . PHP_EOL;

		// Next
		if ($pager->getNextPageURI())
		{
			$links .= '<link rel="prev" href="'. $pager->getNextPageURI() . '">' . PHP_EOL;
		}

		return $links;
	}

	return PHP_EOL . '<link rel="canonical" href="' . \current_url() . '">' . PHP_EOL;
}

@jim-parry jim-parry added the abandoned? Activity on a pull request is almost none since the last interaction with the author label Oct 29, 2018
@natanfelles
Copy link
Contributor Author

Hi, if this feature is not necessary we can close this PR.

@lonnieezell
Copy link
Member

Coming back to this after quite a while. Looking it over again, and thinking it through, I think I'm ok with this feature being in the system. This simply acts as a helper for what you can do anyway ($response->setHeader()) and makes it simpler to use for pagination.

@natanfelles if you want to get this working with the current code base I'm cool with it.

@jim-parry jim-parry changed the title Add Header Link Pagination WIP Add Header Link Pagination Nov 14, 2018
@jim-parry jim-parry removed the abandoned? Activity on a pull request is almost none since the last interaction with the author label Nov 14, 2018
@natanfelles natanfelles force-pushed the response-setHeaderLink branch from 200cf24 to b412d16 Compare November 22, 2018 21:55
@natanfelles natanfelles changed the title WIP Add Header Link Pagination Add Header Link Pagination Nov 22, 2018
@jim-parry jim-parry merged commit a4b9526 into codeigniter4:develop Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants